Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce lint errors / improve TS support #21

Merged
merged 2 commits into from
Jul 13, 2020
Merged

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Jul 8, 2020

Closes #7 ...kind of 😕

Overview

Refactoring the codebase and getting the Wallet tests to cooperate ended up being a much bigger hassle than I anticipated. So in the interest of time and budget, for now we won't be refactoring to enable full TS support.

Instead, this PR removes all lint errors. There are two sources of Typescript lint errors that have been removed.

  • The @typescript-eslint package: These are disabled by section or line when necessary using the appropriate variant of // eslint-disable-this-line
  • The Typescript compiler: These are disabled by line when necessary using // @ts-ignore. A common use case of this is that Typescript doesn't recognize that methods from mixins do in fact exist on the this object, so those are always ignored with @ts-ignore. Reading value from the global state is another common one

Testing this PR

  1. Checkout this branch and run npm install to ensure everything is up to date
  2. Run npm run lint, and nothing should come up aside from an unrelated warning
  3. Run npm run build, and the build should succeed with no errors
  4. Run npm run test:e2e:CI to make sure Cypress e2e tests pass
  5. Run npm run test:unit to make sure Jest unit tests pass
  6. Run npm run dev and ensure the app still works as epxected

Path Forward

As a result of this PR:

  1. We'll have proper TS support in some places, but not others.
  2. We can go back to using the commit and push hooks and stop using the --no-verify flag. This means that during future development we'll need to use the @ts-ignore and eslint-disable-next-line flags as appropriate. Be careful with using the @ts-ignore flag and be sure it's the correct solution (if the app runs as expected, it should be safe to use)

@mds1 mds1 requested review from apbendi and wildmolasses July 8, 2020 23:49
Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mds1, looks good, and your conclusion on how to move forward with this make sense. Thanks for digging into it. Everything works except npm run lint, which I've document in #22. Since it's good otherwise, I'm going to merge, and we can address that separately. Nice work 👍

@apbendi apbendi merged commit bf97c3c into master Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Typescript support in a few places
2 participants